Skip to content

C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414

Open
WolfgangHG wants to merge 3 commits intomicrosoft:mainfrom
WolfgangHG:defaultvalues
Open

C# client: fix default values for DateTime etc, numeric and boolean defaults were not applied#7414
WolfgangHG wants to merge 3 commits intomicrosoft:mainfrom
WolfgangHG:defaultvalues

Conversation

@WolfgangHG
Copy link
Copy Markdown
Contributor

This is an attempt to fix #7404

If the OpenAPI spec contains model fields with format date-time, date, time or uuid and default values, the generated client did not compile because the string value was assigned directly to the property.

Default values for numeric types and boolean were not applied at all, because the code ignored all default values that did not start with a quote.

I also tried to add a test and placed my json file in "Kiota.Builder.IntegrationTests" and added a simple test to "GenerateSample.cs", but only for C#. Other languages apparently handle the default values differently.
This test just runs code generating. It might also make sense to test the generated file. But I don't have an idea how you would do it. It would probably perfect if the client code could be compiled using Roslyn and then the model class could be loaded from the dynamic assembly and the field values could be verified. Or I could take at least a look in the generated class, as e.g. test "GeneratesUritemplateHintsAsync" does. What do you think?

@WolfgangHG WolfgangHG requested a review from a team as a code owner February 24, 2026 19:21
Copy link
Copy Markdown
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

Added a comment to help drive this to completion.
In addition, could you please add unit tests for the code method writer?

Comment thread src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs Outdated
@WolfgangHG
Copy link
Copy Markdown
Contributor Author

In addition, could you please add unit tests for the code method writer?

@baywet The code that I changed is completely covered by GenerateSamples.GeneratesModelWithDefaultValuesAsync - but it is only tested that it does not throw an exception. I could add code to parse the generated file for some specific lines (as is done in other tests in this class).

Could you give me a hint what kind of testing you expect and where I could copy code?

@baywet
Copy link
Copy Markdown
Member

baywet commented Feb 25, 2026

This is essentially an integration test. Thank you for initially adding it.

Did you come across the default value tests in coemethodwritertests? I'm suggesting to add more there.

Let me know if you have additional questions

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

Did you come across the default value tests in coemethodwritertests? I'm suggesting to add more there.

Perfect, that's what I actually was looking for but didn't find ;-).

I added the test for "float value needs the suffix f" to WritesConstructor because it felt correct to place it there - those are all basic tests.
All Parse related tests are placed in a new method WritesConstructorWithDefaultValuesThatRequireParsing

Is the integration test (which parses the full json file) still required? I think so, because it brings the full demo file for this problem.

@baywet
Copy link
Copy Markdown
Member

baywet commented Feb 27, 2026

Thanks for adding the unit tests! I think we're getting really close at this stage.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

I added another small commit. Some integration tests (including my new one) did not set CleanOutput = true, thus the tests would not have re-created the client when running the testsuite multiple times in Visual Studio.

I also experimented a bit (not committed) and added Roslyn compilation to the generated clients of the "Kiota.Builder.IntegrationTests" project. Later I found that there is a big integration tests suite in the "it" directory that is only run during Github pull request verification. Probably, there compilation is also done. But this big test probably does not cover the default value handling of this pull request.

What do you think? Is the approach to compile the "Kiota.Builder.IntegrationTests" clients worth the effort? Or is this unnecessary?

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 2, 2026

What do you think? Is the approach to compile the "Kiota.Builder.IntegrationTests" clients worth the effort? Or is this unnecessary?

Not worth the time. Both in terms of setup, and then in terms of additional runtime of the tests. This is why we set this up as integration tests run by the CI, and not integration tests run as the local test suite. But thank you for considering it.

baywet
baywet previously approved these changes Mar 3, 2026
@WolfgangHG
Copy link
Copy Markdown
Contributor Author

@baywet I see that a lot of tests are failing, most in java clients. Is this caused by my code? Maybe because I detect default values for basic data types now and they are not handled properly when generating the java client?

E.g. here:
https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135366?pr=7414#step:17:256

Comment thread src/Kiota.Builder/KiotaBuilder.cs
@WolfgangHG
Copy link
Copy Markdown
Contributor Author

WolfgangHG commented Mar 4, 2026

I hope I fixed the java problems - a client created from my sample JSON file works again, and the client from https://developers.pipedrive.com/docs/api/v1/openapi.yaml also compiles.

The sample model "WeatherForecast" model looks like this:

public WeatherForecast() {
    this.setBoolValue(true);
    this.setDateOnlyValue(LocalDate.parse("1900-01-01"));
    this.setDateValue(OffsetDateTime.parse("1900-01-01T15:00:00+00:00"));
    this.setDateValueLocalTime(java.time.LocalDateTime.parse("1900-01-01T00:00:00").atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime());
    this.setDecimalValue(25.5d);
    this.setDoubleValue(25.5d);
    this.setFloatValue(25.5f);
    this.setGuidValue(UUID.fromString("00000000-0000-0000-0000-000000000000"));
    this.setSummary("Test");
    this.setTemperatureC(15);
    this.setTimeValue(LocalTime.parse("00:00:00"));
}

Specials compared to C#:

  • double values require "d" suffix always.
  • float values require "f" suffix if they don't contain a decimal point
  • datetime values must have the format DateTimeFormatter.html.ISO_OFFSET_DATE_TIME. In my Swashbuckle generated OpenAPI file, the date had no timezone, which failed to parse in Java. So I add "+00:00" if the default value does not contain a timezone ("+" char).

Update 2026-03-27: after thinking once more about the "datetime without timezone" problem, I understood that ISO8601 allows also datetime values without timezone - thats "local time (unqualified)" (https://en.wikipedia.org/wiki/ISO_8601). OffsetDateTime cannot parse them, it fails.
My solution is to parse them as LocalDateTime, then convert them to an OffsetDateTime in the current timezone of the system. This keeps day/hour/minute unchanged, just applies a timezone:

java.time.LocalDateTime.parse("1900-01-01T00:00:00").atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime()

Now to the other problems - as I don't know Go or Dart, this might be harder...

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

About GO:

Currently, it generates this:

m.SetBoolValue(true)
m.SetDecimalValue(25.5)
m.SetDoubleValue(25.5)
m.SetFloatValue(25.5)

...which results in errors like cannot use true (untyped bool constant) as *bool value in argument to m.SetBoolValue

Following https://www.codegenes.net/blog/how-to-set-bool-pointer-to-true-in-struct-literal/, this could be done like this:

m.SetBoolValue(func() *bool { 
        b := true
        return &b 
    }())

m.SetDecimalValue(func() *float64 { 
        b := 25.5
        return &b 
    }())

m.SetFloatValue(func() *float32 { 
        b := float32(25.5)
        return &b
    }())

The float value literal causes a compilation error, and the float64 value would do the same if the default has no decimal point. Is there any suffix/prefix to define a float32/float64 literal, or must it be done with float32(25.5) or float64(25)?

@baywet What do you think about my suggestion?

By the way: due to my lack of knowledge of theses languages, I would prefer to ignore the date/time default handling hat I added for C#/Java, as they probably never were an issue with GO or Dart previously ;-).

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

Also a PHP error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135791?pr=7414#step:17:139

Error: Parameter #1 $value of class Integration\Test\Client\Repos\Item\Item\Releases\Item\WithRelease_PatchRequestBody_make_latest constructor expects string, true given.
Error: Parameter #1 $value of class Integration\Test\Client\Repos\Item\Item\Releases\ReleasesPostRequestBody_make_latest constructor expects string, true given.

The DART error: https://github.com/microsoft/kiota/actions/runs/22629867053/job/65577135767?pr=7414#step:17:75

Analyzing lib...

  error - models/tweet_create_request.dart:35:33 - Expected to find ';'. - expected_token
  error - models/tweet_create_request.dart:35:38 - Expected an identifier. - missing_identifier
  error - models/tweet_create_request.dart:35:38 - Unexpected text ';'. Try removing the text. - unexpected_token

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 5, 2026

Thanks for looking into this!

For dart maybe we can ask help from @ricardoboss on this.

For go, this is verbose, but I don't have a better solution unless we add short hands in the abstractions library.

As for which types are supported, I believe we should strive for parity between stable languages here.

@ricardoboss
Copy link
Copy Markdown
Contributor

Dart requires default values to be compile time constant, so you can't actually have a default parameter like DateTime.now() or DateTime.parse(...). The parameter must be nullable with null as the default and can then be set using parameter ??= DateTime.now(); (for example).

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

WolfgangHG commented Mar 5, 2026

Making some slow progress with Go and requesting feedback on the current result.

This is the generated Go code for my "WeatherForecast" sample class. I verified that it works and all fields are assigned the expected values.
In contrary to the previous suggestion, I followed the approach already implemented to assign the default value to a variable, then set the pointer to this var to the field.

func NewWeatherForecast()(*WeatherForecast) {
    m := &WeatherForecast{
    }
    boolValueValue := true
    m.SetBoolValue(&boolValueValue)
    dateOnlyValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseDateOnly("1900-01-01")
    m.SetDateOnlyValue(dateOnlyValueValue)
    dateValueValue, _ := i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Parse(i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.RFC3339, "1900-01-01T00:00:00+00:00")
    m.SetDateValue(&dateValueValue)
    decimalValueValue := float64(25.5)
    m.SetDecimalValue(&decimalValueValue)
    doubleValueValue := float64(25.5)
    m.SetDoubleValue(&doubleValueValue)
    floatValueValue := float32(25.5)
    m.SetFloatValue(&floatValueValue)
    guidValueValue, _ := i561e97a8befe7661a44c8f54600992b4207a3a0cf6770e5559949bc276de2e22.Parse("00000000-0000-0000-0000-000000000000")
    m.SetGuidValue(&guidValueValue)
    summaryValue := "Test"
    m.SetSummary(&summaryValue)
    temperatureCValue := int32(15)
    m.SetTemperatureC(&temperatureCValue)
    timeValueValue, _ := i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseTimeOnly("00:00:00")
    m.SetTimeValue(timeValueValue)
    return m
}

Update 2026-03-31: for DateTime values, I now check the default value whether it contains a timezone or not. A value without timezone results fails to parse in RFC3339 format. But it works to do this (this automatically applies the local timezone to the DateTime):

    dateValueLocalTimeValue, _ := i33...d7e.ParseInLocation("2006-01-02T15:04:05", "1900-01-01T00:00:00", i33...d7e.Now().Location())
    m.SetDateValueLocalTime(&dateValueLocalTimeValue)

The first argument ("2006-01-02T15:04:05") is a Go datetime format string, see https://pkg.go.dev/time#pkg-constants. After parsing, the location of the current system time is applied, so that the date value is in the local timezone now.

Attached is the full file:
weather_forecast.go.txt

Question 1: is there anything reusable? I see that the generated GetFieldDeserializers calls a lot of methods like ParseNode.GetDateOnlyValue. Could the code generator somehow generate calls to this class? If yes: how to parse a string value?
Question 2: all Parse methods return a second error object. My sample snippet just discards it. How to handle it?

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 6, 2026

Thank you for the additional information.

Yes, I like this approach! It's verbose, but consistent at least.

Q1: I don't think you'll be able to easily reuse the parse node infrastructure because that not only requires you to have access to the parse node factories, but also get bytes from string, and the representation might vary based on the mime type/available implementation. All of which add layers of complexity that calling a static factory is much simpler. Maybe it'd be worth refactoring things to make sure the parts that emit those lines are defined once and not duplicated.

Q2: I've always found that part to be odd in Go's design. Essentially "constructors" don't typically return an error, but the way errors work, they require an extra return value. It does not fit well. Since we don't want to introduce breaking changes, this effectively leaves us with two choices:

  • panic on error
  • swallow/ignore the errors

My opinion is that failing to set a default because the spec is odd, or because the static factory fails to parse a value, is not a huge deal which should lead the application to crash. So panicking is probably too aggressive. Of course that's something that can always be updated later on if people have issues with errors being ignored.

Let me know if you have any additional comments or questions.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

Here we go with the Go client.

Hope I didn't break it - not having seen Go before three days ago...

@baywet Thanks for the feedback about error handling. I agree completely - a parse error while applying the defaults should be avoided. If a user runs into a parse problem, she could still add error logging in the generated code.

I am very confused that the Go test CodeMethodWriterTests.WritesConstructorWithEnumValue expected a line SetSize(TENTWENTYFOUR_BY_TENTWENTYFOUR) that should not have compiled in reality as the setter is generated so that the enum parameter is a pointer to an enum value. And this test did not fail before I starting changing code generation. Seems someone wrote a test for a non-compiling method.

Assert.Contains($"Set{propName.ToFirstCharacterUpperCase()}({defaultValue})", result);//ensure symbol is cleaned up

@baywet Could you start the Github CI test run so that I can see whether my changes fix the failing Java and Go tests?

PHP and Dart are still on my TODO list, but not before next Tuesday. I also don't know those two languages ;-)

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

Are those GO errors also caused by me?

For example https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047630?pr=7414#step:17:35:

Error: client/models/component1.go:23:25: cannot use &additionalDataValue (value of type *map[string]any) as map[string]any value in argument to m.SetAdditionalData

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 9, 2026

Are those GO errors also caused by me?

For example https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047630?pr=7414#step:17:35:

Error: client/models/component1.go:23:25: cannot use &additionalDataValue (value of type *map[string]any) as map[string]any value in argument to m.SetAdditionalData

I'd say yes since one of the files it's erroring with is local (not pulled from an external repo) and since this is only happening on this branch.

Let us know if you have any additional comments or questions.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

Yes, it was me ;-). Previously, a default value was always set as m.setProperty(defaultValue), except if the default value started with a quote. In this case, it was first assigned to a variable, then the setter was invoked as m.setProperty(&defaultValue).
Since my change, also default values not starting with a quote are detected (numeric values, boolean), thus I removed the check "default value starts with quote" and handle all those default values the same way. Because of this change, the code for additionalData assignd the default to a variable and called the setter with a reference.

With the latest commit, I brought back the old behavior:

m.SetAdditionalData(make(map[string]any))

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

@baywet There is one failing Java test:

https://github.com/microsoft/kiota/actions/runs/22796339086/job/66295047215?pr=7414#step:17:201

The error is:
WithReleasePatchRequestBody.java:[53,75] incompatible types: boolean cannot be converted to java.lang.String

The reason is this snippet:

  "make_latest": {
    "default": true,
    "description": "Specifies whether this release should be set as the latest release for the repository. ....",
    "enum": [
      "true",
      "false",
      "legacy"
    ],

Shouldn't the default for the enum value be placed in quotes here? Before, it was not detected.

Kiota now creates this java snippet:

public WithReleasePatchRequestBody() {
    this.setAdditionalData(new HashMap<>());
    this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue(true));
}

forValue expects a string.

I could work around by adding quotes to enum default values if they are missing:

if (propWithDefault.Type is CodeType propertyType && propertyType.TypeDefinition is CodeEnum enumDefinition)
{
     if (!defaultValue.StartsWith('"'))
     {
         defaultValue = $"\"{defaultValue}\"";
     }
    defaultValue = $"{enumDefinition.Name}.forValue({defaultValue})";
}

How to continue? Add the workaround? Or if my understanding is correct and the syntax is invalid, would it be worth a bug report?

Could you also trigger another run of the tests to verify that the Go problems are fixed?

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 10, 2026

In this example, the default value (in the OpenAPI description) is wrong IMHO.

That's because the enum value is "true" (a string) but the default is true (a boolean). And the default would not validate against the enum keyword according to JSON schema only.

I'd be ok suppressing the specific operation that's bringing this type in the configuration here for that reason.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

I created github/rest-api-description#6100. Seems they fixed one of the problems already. But I am a bit irritated because the file that Kiota downloads differs a lot from the github file. Did I look at the proper file ;-)?

@baywet Why not use the approach of the C# client generator instead of calling a parsing method?

if (propWithDefault.Type is CodeType propertyType && propertyType.TypeDefinition is CodeEnum)
{
defaultValue = $"{conventions.GetTypeString(propWithDefault.Type, currentMethod).TrimEnd('?')}.{defaultValue.Trim('"').CleanupSymbolName().ToFirstCharacterUpperCase()}";
}

Currently generated Java code:

public WithReleasePatchRequestBody() {
    this.setAdditionalData(new HashMap<>());
    this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.forValue("true"));
}

The C# approach would result in this code:

public WithReleasePatchRequestBody() {
    this.setAdditionalData(new HashMap<>());
    this.setMakeLatest(WithReleasePatchRequestBodyMakeLatest.True);
}

Here, the generated code compiles without errors. Could we use this code?

By the way: the "config.json" that you linked contain some suppressions for apisguru::github.com:api.github.com that link to fixed issues. They could probably restored?

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 11, 2026

Seems they fixed one of the problems already. But I am a bit irritated because the file that Kiota downloads differs a lot from the github file. Did I look at the proper file ;-)?

Yes, most descriptions come from the APIs.guru index. Which is often out of date sadly. This is a bit outside of the scope of this pull request but you could do a could of things:

  • reach out to apis.guru and tell them their sync is broken/behind for github
  • reach out to github and tell them to add their description to the "github based index" https://learn.microsoft.com/en-us/openapi/kiota/add-api (+ add an entry in the APIs guru search provider here to exclude github descriptions and avoid duplicates)
  • update the test configuration to use that description instead of the APIs.guru one.

In any case, if any changes are required in kiota's repo, please open a separate pull request to avoid mixing concerns/spead up the review process.

Why not use the approach of the C# client generator instead of calling a parsing method?

I'm not sure/can't remember why we'd have a difference in the approach between languages here. Could be as simple as the initial language implementers not coordinating. Happy to see things getting aligned assuming it doesn't introduce regressions.

to fixed issues. They could probably restored?

yes, it's probably an oversight. Please do that in a separate PR though.

Thanks for the continuous work here. Let me know if you have any additional comments or questions.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

About a ruby mockserver test: I did not even touch the ruby part of Kiota, as there were no errors: the generated code just assigned the string defaults, and there are no datatype definitions for properties. But there is probably a difference beetween

@date_only_value = "1900-01-01"

and

@date_only_value = Date.parse("1900-01-01")

So, I will try to parse the default values based on the data types returned by the JsonParseNode

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

WolfgangHG commented Apr 15, 2026

I committed changes to Ruby and added a MockServer test (that did not exist before).

Before, it generated this code:

def initialize()
    @bool_value = true
    @date_only_value = "1900-01-01"
    @date_value = "1900-01-01T00:00:00+00:00"
    @date_value_local_time = "1900-01-01T00:00:00"
    @decimal_value = 25.5
   ...
    @enum_value = "one"
    ...
    @guid_value = "00000000-0000-0000-0000-000000000000"
    ...
    @time_value = "00:00:00"
end

Now it looks like this:

def initialize()
    @bool_value = true
    @date_only_value = Date.parse("1900-01-01")
    @date_value = DateTime.parse("1900-01-01T00:00:00+00:00")
    @date_value_local_time = DateTime.parse("1900-01-01T00:00:00")
    @decimal_value = 25.5
    ...
    @enum_value = Integration_test::Client::Models::WeatherForecastEnumValue[:One]
    ...
    @guid_value = UUIDTools::UUID.parse("00000000-0000-0000-0000-000000000000")
    ...
    @time_value = Time.parse("00:00:00")
end

Parsing of DateTime defaults without timezone ("local time") actually does not work as expected - it creates an UTC value.
It seems there is no easy way to parse it in the current timezone. It could be possible with this code snippet:

@date_value_local_time = Time.local(2026, 6, 1, 13, 0, 5, 0).to_datetime

But the method local requires a lot of single parameters (see https://ruby-doc.org/3.4.1/Time.html#method-c-local), so it is not possible to parse a datetime string and call the local method in a single line.
It might work with ActiveSupport extensions and the change method.

Note that I also changed the handling of the enum value, which assigned a string before. Looking at the data that was deserialized from the Mockserver Json reponse, I saw that the Json data was also parsed as a real enum value, so I tried to change the code the same way (copied from other writers ;-))

=====================================

All other Mockserver tests have a directory "basic" that I treated as a template for new tests. Here, the service invocation for "InheritingErrors.yaml" resulted in a compilation error:

Error: uninitialized constant Integration_test::Client::Api::V1::Topics::TopicsRequestBuilder::Binary

It is caused by this line in "kiota\it\ruby\lib\integration_test\client\api\v1\topics\topics_request_builder.rb":

return @request_adapter.send_async(request_info, Binary, error_mapping)

So the "basic" test file has a comment line for the service invocation.

I wonder why this action before did not report errors (latest run: https://github.com/microsoft/kiota/actions/runs/24415122058/job/71353398995). Does ruby not compile the full generated code?

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

This pull request is again ready for review and for merging. I worked around microsoft/kiota-abstractions-ruby#82 by creating a custom AnonymousAuthenticationProvider in my ruby test. I will try to create a pull request for this fix, and if a new ruby abstractions release is done, I will revert my workaround. But this can happen later.

Copy link
Copy Markdown
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great continuous work on this. Left a comment here. I'll also ask copilot to review. I've designed a skill for the sanitation, hopefully it catches things I've missed. We also have failing CI to look into. To give you an idea of progress, I've reviewed 54 out of 55 changed files so far.

Comment thread src/Kiota.Builder/Writers/Go/CodeMethodWriter.cs

This comment was marked as outdated.

@WolfgangHG
Copy link
Copy Markdown
Contributor Author

I fixed all comments besides one - see my comment on your feedback. CI should also work again. It was a dumb formatting issue - the last change before committing - it sounded so trivial that no further test run seemed to be required ;-)

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/Kiota.Builder.Tests/Writers/Php/CodeMethodWriterTests.cs Outdated
Comment on lines +332 to +336
//OffsetDateTime can only parse a date string that contains a timezone or "Z" (format "DateTimeFormatter.ISO_OFFSET_DATE_TIME").
"offsetdatetime" when defaultValue.Contains('+', StringComparison.InvariantCulture) || defaultValue.Contains('Z', StringComparison.OrdinalIgnoreCase) => $"OffsetDateTime.parse({defaultValue})",
//Otherwise: consider it a local date/time value. Create a OffsetDateDateTime in the current timezone.
"offsetdatetime" => $"java.time.LocalDateTime.parse({defaultValue}).atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime()",
"localtime" => $"LocalTime.parse({defaultValue})",
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timezone detection for OffsetDateTime defaults only checks for '+' or 'Z'. RFC3339 offsets can also be negative (e.g. ...-05:00), which would incorrectly fall into the LocalDateTime.parse() branch and throw at runtime. Consider detecting an offset more robustly (e.g. regex /[+-]\d{2}:\d{2}$/ or DateTimeFormatter parsing attempt) so negative offsets are handled by OffsetDateTime.parse().

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@WolfgangHG WolfgangHG Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot is correct. I extracted the code to a helper method StringExtensions.IsDateTimeWithOffset, where I detect whether the string ends with "Z" or matches the regex suggested by Copilot.
There is also a test for this method.

@baywet Could you review this? Is there a better place for such a method?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kiota's general direction on those kind of issues has been to avoid patching for bad descriptions. Because it's a rabbit hole with infinite possibilities.

If we get a offsetdatetime type at this place it's because the description type was string and format was date. To enable interoperability, the registry suggests this should comply with RFC3339 date-time, which is mandated to contain an offset (Z or numerical).

If the corresponding default value matches this, then the equivalent Java (or any language) API should be able to parse it without any issues.
If the corresponding default value DOES NOT match this, then the description is at worst not interoperable, at best inconsistent with itself.
In both cases, this is not something that kiota should cater to, there are too many possibilities with such issues, it'd make things unmaintainable.

My suggestion is to drop the parsing method you added, drop the when qualifier on the first offsetdatetime case, and then drop the second offsetdatetime case entirely.

If an integration test is failing because of that, we can suppress it as you already know the mechanism for that.

Let me know if you have any additional comments or questions.

Comment thread src/Kiota.Builder/Writers/Go/CodeMethodWriter.cs Outdated
Comment thread src/Kiota.Builder/Refiners/TypeScriptRefiner.cs
Comment thread src/Kiota.Builder/Refiners/TypeScriptRefiner.cs
@WolfgangHG
Copy link
Copy Markdown
Contributor Author

I fixed the failing CI test (ai detected it also ;-)), and I will think about the rest and continue tomorrow. Detecting wether a datetime default value has a timezone or not might be more difficult than I thought.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Kiota.Builder/Writers/Go/CodeMethodWriter.cs Outdated
Comment on lines +2148 to +2152
// The hostile payload's inner quotes must be escaped so it stays inside the string literal
Assert.Contains(@"DateTimeOffset.Parse(""1900-01-01\""); Evil.Run(\""x"").Date)", result);
// String default must have newline and quotes escaped
Assert.Contains(@"""line1\""", result);
Assert.Contains(@"\n", result);
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hostile-payload assertion appears to expect the injected \"); Evil.Run(...) sequence to break out of the string literal (note the \""/double-quote sequence). With SanitizeQuotedStringLiteral escaping inner quotes, the generated code should keep Evil.Run inside the string argument; this assertion is likely incorrect/brittle and may fail (or pass while still allowing injection). Consider asserting that the entire payload remains within a single string literal (e.g., Evil.Run only appears inside the parsed string argument) rather than matching a hard-coded escaped substring.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baywet the WritesConstructorWithDefaultValuesSanitizesHostilePayloads was added by you in this commit.

Could you comment on the Copilot comment?
I think the expected string DateTimeOffset.Parse("1900-01-01\"); Evil.Run(\"x").Date) is perfectly fine.

I planned to squash the commits again, but I will delay it until this is sorted out, just to keep the commit history traceable.

By the way: thanks for taking care of sanitation. This is really a bad coincidence that my pull requests might open a window where you closed a lot of doors ;-)

Comment thread it/php/.gitignore
Comment on lines +1 to +5
#generated api code if integration test is run locally
src/client/

vendor
composer.lock No newline at end of file
composer.lock
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignore list covers src/client/, but it/exec-cmd.ps1 copies generated PHP code into src/Client/ (capital C) for MockServer tests. On case-sensitive filesystems (Linux CI), src/Client/ won't be ignored and will show up as untracked changes. Consider ignoring both casings (or aligning the copy destination with the ignored path).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably correct, though I don't know whether development is done on Linux.

I also changed the client path to "src\Client" in "get-additional-arguments.ps1", as it turned out in my "defaultvalues" test that the CI fails to find classes if the namespace from "composer.json" is upper case, but a subdir is lower case (see #7414).
But this problem only occured in unit test files, so it is just code consistency without effect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's assume contributors come for a wide variety of OSes. And windows is Ci anyway, to matching the case we expect from our own scripts is the correct path forward here.

Copy link
Copy Markdown
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the continuous work here. And sorry about the delay in review lately. Having some health challenges on my end.

public static bool IsDateTimeWithOffset(this string dateTime)
{
if (string.IsNullOrEmpty(dateTime)) return false;
return dateTime.EndsWith('Z') || dateTime.EndsWith('z') || Regex.IsMatch(dateTime, "[+-]\\d{2}:\\d{2}$");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's revert that change, we shouldn't get in the business of validating default values.

Comment on lines +332 to +336
//OffsetDateTime can only parse a date string that contains a timezone or "Z" (format "DateTimeFormatter.ISO_OFFSET_DATE_TIME").
"offsetdatetime" when defaultValue.Contains('+', StringComparison.InvariantCulture) || defaultValue.Contains('Z', StringComparison.OrdinalIgnoreCase) => $"OffsetDateTime.parse({defaultValue})",
//Otherwise: consider it a local date/time value. Create a OffsetDateDateTime in the current timezone.
"offsetdatetime" => $"java.time.LocalDateTime.parse({defaultValue}).atZone(java.time.ZoneId.systemDefault()).toOffsetDateTime()",
"localtime" => $"LocalTime.parse({defaultValue})",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kiota's general direction on those kind of issues has been to avoid patching for bad descriptions. Because it's a rabbit hole with infinite possibilities.

If we get a offsetdatetime type at this place it's because the description type was string and format was date. To enable interoperability, the registry suggests this should comply with RFC3339 date-time, which is mandated to contain an offset (Z or numerical).

If the corresponding default value matches this, then the equivalent Java (or any language) API should be able to parse it without any issues.
If the corresponding default value DOES NOT match this, then the description is at worst not interoperable, at best inconsistent with itself.
In both cases, this is not something that kiota should cater to, there are too many possibilities with such issues, it'd make things unmaintainable.

My suggestion is to drop the parsing method you added, drop the when qualifier on the first offsetdatetime case, and then drop the second offsetdatetime case entirely.

If an integration test is failing because of that, we can suppress it as you already know the mechanism for that.

Let me know if you have any additional comments or questions.

Comment thread it/php/.gitignore
Comment on lines +1 to +5
#generated api code if integration test is run locally
src/client/

vendor
composer.lock No newline at end of file
composer.lock
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's assume contributors come for a wide variety of OSes. And windows is Ci anyway, to matching the case we expect from our own scripts is the correct path forward here.

[Fact]
public void DetectsDateTimeOffset()
{
Assert.True("1900-01-01T00:00:00+00:00".IsDateTimeWithOffset());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be reverted as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, two issues became part of the same conversation. This might get complicated.

First the easier one: my latest change from "client" to "Client" (as suggested by the AI) breaks CI. I will have to fix ".gitignore" and the powershell script. You wrote "And windows is Ci anyway" - did you mean the Github CI? But it runs with Ubuntu.

Second is the (local) datetime issue: The link you provided mentions two date time types:

  • date-time-local RFC3339 date-time without the timezone component
  • date-time date and time as defined by date-time - RFC3339

"date-time-local" is probably bad design, but it works if you know that your service is only used e.g. by German customers (as happens for our own REST service).
As I didn't have to care for timezones, I used the .NET "DateTime" type for properties in our own service (with "DateTimeKind = Unknown"), and the Swashbuckle layer generated a OpenAPI spec from my controllers and classes, also using datetime strings without timezone.
During the work on this pull request, I asked in the Swashbuckle forum and they gave me a workaround to use a JsonConverter to set a "DateTimeKind = Local", which would result in DateTime values being serialized with the local timezone.

Supporting "date-time-local" might be a bigger problem, as Kiota does not seem to support this type at all.

I found this issue https://www.github.com/OAI/OpenAPI-Specification/issues/4759, and you probably know it as you reviewed the corresponding pull request ;-).

The code in this pull request that checks whether a DateTime value has a timezone or not is mainly a workaround for my own badly designed service ;-). I could fix it to use proper timezone values with the help of a JsonConverter, but I would have to keep it backwards compatible for existing customers that don't want to fix their clients.

Let's continue this tomorrow, it is late now...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether it is allowed by RF3339, but my interpretation was up to now that a datetime without timezone is in local timezone. But maybe I got confused and took this information from ISO8601 where it is allowed.
Some languages (C# and DateTimeOffset) seem to handle this more lenient, others (Java, Go) don't and raise an exception. That's why I added fallback code to parse a datetime string without timezone.

What do you think? Shall I revert the fallback changes and require "datetime with timezone" values always?

This would also mean I have to fix my own service in a version "2" ;-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why we're having both conversations here. By CI I meant case insensitive. Sorry for the confusion

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to date-time, not the local variant to be clear.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert the fallback for now and see if people complain about this edge case, which I suspect they won't.

(Sorry for the fragmented reply, I'm on my phone)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hopefully fixed the failing PHP test in CI ("continous integration", not "case insensitive" ;-)).

Reason for the change: when I added a PHP integration test, it failed in the Github CI (linux) because it couldn't find classes in namespace "Client..." - the directory was named "client". This was fixed in "exec-cmd.ps1" which copied the generated client from "client" to a dir "Client" in the integration test folder.
Copilot suggested to change the casing also for the "client" directory where the generated client is written to. This broke CI.

Will revert the "local date time" handling the next few days.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code for "datetime without timezone" was reverted. Pull request is again ready for review.

…ric and boolean defaults were not applied, add mockserver tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default values for DateTime/Date/Guid result in non-compileable code

6 participants